[Toolbar] Hover style and Toggle State [ATL-1107][ATL-1045] #190
Conversation
fstasi
changed the title
| @@ -65,7 +71,17 @@ export class VerifySketch extends SketchContribution { | |||
| } | |||
|
|
|||
| async verifySketch(exportBinaries?: boolean): Promise<void> { | |||
|
|
|||
| // even with buttons disabled, better to double check if a verify is already in progress | |||
There was a problem hiding this comment.
better to double check
Why? This code should not run if isEnabled is implemented correctly for the command handler. Did you experience a bug?
There was a problem hiding this comment.
I added this check in order to future-proof the verify-sketch implementation:
https://arduino.atlassian.net/browse/ATL-1045 reports an error (Error: Request upload failed with message: 2 UNKNOWN: exit status 1) when a verify/build is run while another is in progress.
At the moment we only have buttons/menu items, but the check whether or not continue with the action should be inside the action handler itself in my opinion..
Do you have concerns about this approach?
| @@ -18,15 +18,21 @@ export class VerifySketch extends SketchContribution { | |||
| @inject(BoardsServiceProvider) | |||
| protected readonly boardsServiceClientImpl: BoardsServiceProvider; | |||
|
|
|||
| verifyInProgress = false; | |||
There was a problem hiding this comment.
This should be a protected field, not a public mutable property. Same for the upload part.
| .p-TabBar-toolbar .item.arduino-tool-item > div:hover { | ||
| background: (--theia-arduino-toolbar-hoverBackground); | ||
| .p-TabBar-toolbar .item.arduino-tool-item:hover > div { | ||
| background: white; |
There was a problem hiding this comment.
We cannot use hard-coded color so neither white nor rgb(255,204,0) is acceptable. We use VS Code themes, any new colors we want to use have to be derived from the available theme colors.
- https://code.visualstudio.com/docs/getstarted/themes
- https://code.visualstudio.com/api/extension-capabilities/theming
- https://code.visualstudio.com/api/references/theme-color
Here is the Theia doc: https://github.com/eclipse-theia/theia/pull/6475/files#diff-06572a96a58dc510037d5efa622f9bec8519bc1beab13c9f251e97e657a9d4ed
Is that ok to squash merge the PR in order to have a single commit on master? Or do you mean you want a single commit in the PR? |
Why
How
isToggledfunction of the registry command was previously not called by Toolbar Items. I adopted the same mechanism used forisEnabled.Other
Jira Task (cosmetics)
Jira Task (disable while in progress)
Fixes #173